Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve unit tests #2391

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Improve unit tests #2391

wants to merge 5 commits into from

Conversation

guusbertens
Copy link

@guusbertens guusbertens commented Dec 6, 2024

Proposed Changes

  • Upgrade to Meson 0.64.1. This minor version introduces fs.copyfile, which is needed for copying files for unit tests (see next point). The latest patch level within this minor version is 0.64.1.
  • Fix the build system so it copies files that are needed for unit tests to the build dir.
  • Do not install unit tests. Unit tests depend on files that are not currently installed, so if the tests were installed, they could not run anyway.

Related Work

  • No currently open issues mention "unittests"
  • No currently open PRs mention "unittests"

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

Githooks man page says that the pre-commit hook must have run, but I don't know how to verify that it did.

Guus Bertens added 3 commits December 9, 2024 00:15
Meson 0.64.0 introduces fs.copyfile().  We use the latest patch level of
0.64, which 0.64.1.
endif

if get_option('enable-autodiff')
unit_test_files_ad = su2_cfd_tests_ad + files(['test_driver.cpp'])
test_driver_AD = executable(
'test_driver_AD',
unit_test_files_ad,
install : true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to change the regression workflow to find the unit test binaries somewhere else. It's simpler if you add a meson option to skip installing them by default, and then we can install them in the regression test script.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll have a look!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... The documentation indicates the unit tests should be run from within the build dir (https://su2code.github.io/docs_v7/Running-Unit-Tests/). Shouldn't the regressions test script do the same, then?

(I'm not an SU2 pro... Maybe the regression test script tests for more and/or other things, which would invalidate the above.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, there are multiple tests being run:

  • UnitTests/test_driver{,_AD,_DD}
  • TestCases/...

The former currently relies on hardcoded paths in the source tree, which typically only work in the build container and test container. The hardcoded paths break the unit tests in any other situation.

My patches remove the hardcoded paths, and instead tell Meson to move the files to the build tree. This makes the unit tests work as documented.

Of course, the files can be installed as well. The test_driver then needs to be run from the right dir, or needs to have correct paths hardcoded into it. In the latter case, the documentation needs an update: only run the tests after installing.

So, there are three options:

  1. Do nothing: the documentation should be changed to reflect that the unit tests will generally not work, unless run under very specific circumstances.
  2. Remove hardcoded paths, do not install unit tests, run unit tests at build time. This is how it is documented, and how most other software does it.
  3. Remove hardcoded paths, install unit tests, at test time go to correct work dir and run unit tests. Documentation needs an update too.
  4. Have build system insert hardcoded paths to $prefix, install unit tests, run unit tests at test time. Documentation needs an update too.

3 and 4 would break things like EasyBuild, that assume tests are run before installation, and require an update to the documentation.

2 seems like the right option.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option 2 would require a change to the compileSU2.sh script. I doubt that that will be reflected anytime soon, which results in the tests failing until the container is updated.

@pcarruscag I think options 3 above is what you suggested, but would still either break the documented way to run unit tests (ninja -C builddir test), or require one to specify a workdir in regression.yml, which seems impossible.

What is the intention of tests? Should they be run before installing, to ensure one doesn't install a broken binary, or should they be run after installing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants